feat(axis): small multiples axis improvements #1004

nickofthyme · 2021-02-02T02:30:21Z

Summary

fixes #985, fixes #986

Related to #891

This PR adds functionality around the axis for small multiples including the following...

Allow a single main axis to spread the length of all panels defined by Axis.title
Enable axis title for each panel with own style AxisPanelTitle
Show axis lines on all non-primary (aka secondary) axes
Add title prop to GroupBy for formatting panel titles by value
Simplify title dimensioning logic
Fix check for small multiples with single bucket issue related to this code.
Fix bug with sm value of zero, where a falsy smV or smH value would not render the series correctly.

Demo

Checklist

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios

- Add panel axis for sm chart with own style - Add title prop to GroupBy for formatting sm titles by panel - DRY up repeated types - Simplify title dimensioning logic - Fix check for sm with single bucket - Fix bug with sm value of zero

- Add options to grid story - Hide title when non is defined or empty string

codecov-io · 2021-02-02T23:54:14Z

Codecov Report

Merging #1004 (ddf5496) into master (4c8f46c) will decrease coverage by 0.37%.
The diff coverage is 43.45%.

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
- Coverage   72.48%   72.11%   -0.38%     
==========================================
  Files         353      371      +18     
  Lines       11063    10776     -287     
  Branches     2419     2221     -198     
==========================================
- Hits         8019     7771     -248     
+ Misses       3029     2904     -125     
- Partials       15      101      +86

Flag	Coverage Δ
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
..._types/xy_chart/renderer/canvas/primitives/text.ts	`7.14% <ø> (+0.07%)`	⬆️
src/chart_types/xy_chart/utils/grid_lines.ts	`85.00% <ø> (-3.38%)`	⬇️
src/specs/group_by.ts	`100.00% <ø> (ø)`
src/utils/themes/dark_theme.ts	`100.00% <ø> (ø)`
src/utils/themes/light_theme.ts	`100.00% <ø> (ø)`
src/utils/themes/theme.ts	`100.00% <ø> (ø)`
...ypes/xy_chart/renderer/canvas/axes/global_title.ts	`16.32% <13.63%> (ø)`
...types/xy_chart/renderer/canvas/axes/panel_title.ts	`15.38% <15.38%> (ø)`
src/utils/common.ts	`86.57% <20.00%> (-3.84%)`	⬇️
...chart_types/xy_chart/renderer/canvas/axes/index.ts	`30.90% <21.05%> (+0.47%)`	⬆️
... and 226 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c8f46c...ddf5496. Read the comment docs.

monfera · 2021-02-03T12:53:40Z

Good additions and improvements here!

Allow a single main axis to spread the length of all panels defined by Axis.title

It refers to a single text label only, ie. not a full axis, so no line, ticks and tick values, correct? Though I guess the per row category labels can be seen as tick values of the outside axis

Related: it'd be neat to have a knob for toggling the title="Metric"; eg. it can be an image test to ensure that when deactivated, there's no space reserved for it, which is very nice.

nickofthyme · 2021-02-03T15:31:55Z

Allow a single main axis to spread the length of all panels defined by Axis.title

It refers to a single text label only, ie. not a full axis, so no line, ticks and tick values, correct? Though I guess the per row category labels can be seen as tick values of the outside axis

Not sure if I fully understand your question but I think yes. It treats the AxisSpec.title as a static label that spans the full width or height. Then if there are panel axes it will insert them between. The ticks are somewhat independent from either of the titles. So there are no changes to the existing non-small-multiple charts or axes. Below is toggling on and off the horizontal small multiple accessor, notice the panel title just disappears.

Related: it'd be neat to have a knob for toggling the title="Metric"; eg. it can be an image test to ensure that when deactivated, there's no space reserved for it, which is very nice.

Great idea! I forgot to add some vrt for this. I'll do that now.

monfera

Looks great! A couple of smaller optional changes linked to rows

stories/small_multiples/2_vertical_areas.tsx

api/charts.api.md

src/chart_types/xy_chart/renderer/canvas/axes/global_title.ts

src/chart_types/xy_chart/renderer/canvas/axes/panel_title.ts

src/chart_types/xy_chart/utils/axis_type_utils.ts

src/specs/group_by.ts

src/utils/common.ts

src/chart_types/xy_chart/renderer/canvas/axes/panel_title.ts

markov00

Left few comments, in particular I'd like to discuss the title prop

api/charts.api.md

src/chart_types/xy_chart/utils/series.ts

markov00 · 2021-02-04T15:45:40Z

src/specs/group_by.ts

+   * If both axis title and this value are set, the axis title will be
+   * treated as the primary title and this as the secondary.
+   */
+  title?: GroupByTitleFormatter;


The GroupBy component is a general component and it can be reused for much more than just creating small multiples.
The title prop implies that some text will be rendered somewhere.
I prefer something like lookup or format that describes an action we are doing to each of the aggregated keys.

Also we should be carefully describe when this formatting take place: before the sorting or after that, because it can really change the way sorting works.

markov00 · 2021-02-04T15:47:57Z

stories/small_multiples/4_horizontal_bars.tsx

-      <Axis id="time" title="metric" position={Position.Bottom} gridLine={{ visible: false }} />
-      <Axis id="y" title="Day of week" position={Position.Left} gridLine={{ visible: false }} />
+      <Axis id="time" position={Position.Bottom} gridLine={{ visible: false }} />
+      <Axis id="y" title="Day of week" position={Position.Right} gridLine={{ visible: false }} />


can we go back to the Left position? bar charts should always have a baseline

# [24.6.0](v24.5.1...v24.6.0) (2021-02-15) ### Bug Fixes * **legend:** width with scroll bar ([#1019](#1019)) ([45bd0d5](45bd0d5)) ### Features * sort values in actions by closest to cursor ([#1023](#1023)) ([e1da4e5](e1da4e5)) * **axis:** small multiples axis improvements ([#1004](#1004)) ([514466f](514466f)) * **partition:** drilldown ([#995](#995)) ([20bbdae](20bbdae))

markov00 · 2021-02-15T10:49:45Z

🎉 This PR is included in version 24.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

nickofthyme added enhancement New feature or request :axis Axis related issue :xy Bar/Line/Area chart related :small multiples Small multiples/trellising related issues labels Feb 2, 2021

nickofthyme requested a review from markov00 February 2, 2021 02:30

nickofthyme added 3 commits February 1, 2021 20:30

fix: issue with axes without panel titles

96bbe86

feat: show axis lines on all secondary sm axes

136d6d2

fix: spacing based on positions

8bdeb19

- Add options to grid story - Hide title when non is defined or empty string

nickofthyme force-pushed the sm-axis-improvements branch from 50410cd to 8bdeb19 Compare February 2, 2021 02:31

nickofthyme added 4 commits February 1, 2021 20:55

fix: update image vrt

79f67bf

fix: bad argument order to getAxisPosition

4b42616

fix: update unit tests based on latest changes

6562cbc

fix: update unit tests type changes

7561a00

Base automatically changed from master to masterx February 2, 2021 23:10

Base automatically changed from masterx to master February 2, 2021 23:28

Merge branch 'master' into sm-axis-improvements

5a7e407

fix: types and api changes

6c457d5

test: add tests for new panel positioning

12429e0

monfera approved these changes Feb 3, 2021

View reviewed changes

nickofthyme added 3 commits February 3, 2021 10:58

fix: title visibility toggle

553ecde

test: add vrt screenshots to test new sm axes

2a2f8bc

fix: pr comments to document types

7155cf6

nickofthyme commented Feb 4, 2021

View reviewed changes

src/chart_types/xy_chart/renderer/canvas/axes/panel_title.ts Outdated Show resolved Hide resolved

fix: simplify conditional return

67db527

markov00 reviewed Feb 4, 2021

View reviewed changes

fix: change title to format, fix axis position

60ab456

nickofthyme added 4 commits February 5, 2021 10:22

Merge branch 'master' into sm-axis-improvements

e5f3f12

fix: add yarn changes

ddf5496

fix: api changes

fcc9b8f

fix: update screenshot

5b573a1

nickofthyme enabled auto-merge (squash) February 5, 2021 17:23

nickofthyme merged commit 514466f into elastic:master Feb 5, 2021

nickofthyme deleted the sm-axis-improvements branch February 5, 2021 19:51

markov00 added the released Issue released publicly label Feb 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(axis): small multiples axis improvements #1004

feat(axis): small multiples axis improvements #1004

nickofthyme commented Feb 2, 2021 •

edited

Loading

codecov-io commented Feb 2, 2021 •

edited

Loading

monfera commented Feb 3, 2021 •

edited

Loading

nickofthyme commented Feb 3, 2021

monfera left a comment

markov00 left a comment

markov00 Feb 4, 2021

nickofthyme Feb 5, 2021

markov00 Feb 4, 2021

nickofthyme Feb 5, 2021

markov00 commented Feb 15, 2021

feat(axis): small multiples axis improvements #1004

feat(axis): small multiples axis improvements #1004

Conversation

nickofthyme commented Feb 2, 2021 • edited Loading

Summary

Demo

Checklist

codecov-io commented Feb 2, 2021 • edited Loading

Codecov Report

monfera commented Feb 3, 2021 • edited Loading

nickofthyme commented Feb 3, 2021

monfera left a comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

markov00 Feb 4, 2021

Choose a reason for hiding this comment

nickofthyme Feb 5, 2021

Choose a reason for hiding this comment

markov00 Feb 4, 2021

Choose a reason for hiding this comment

nickofthyme Feb 5, 2021

Choose a reason for hiding this comment

markov00 commented Feb 15, 2021

nickofthyme commented Feb 2, 2021 •

edited

Loading

codecov-io commented Feb 2, 2021 •

edited

Loading

monfera commented Feb 3, 2021 •

edited

Loading